fix `config.protectAllJoinedRooms` leaking into explicitly protected rooms #385

Gnuxie · 2022-10-14T19:10:14Z

Closes #370
Covers all of the points we wanted to refactor on in #370 too.

This PR factors out the management of explicitlyProtectedRoomIds into a ProtectedRoomsConfig class which also manages persisting the list to account data.
It then nukes protectedJoinedRoomIds, unprotectedWatchedListRooms and explicitlyProtectedRoomIds from Mjolnir.
If config.protectAllJoinedRooms is true, rather than tracking which policy lists should not be protected based on the create event of the list (which is what unprotectedWatchedListRooms was for), we instead require that lists be explicitly protected. This will already be the case if the list was created with Mjolnir.
To make sure it is obvious that a list is unprotected, the status command will also distinguish between watched and protected lists.

This in turn makes the code around resyncJoinedRooms that implements config.protectAllJoinedRooms much much simpler.

ShadowJonathan · 2022-10-17T11:53:49Z

src/Mjolnir.ts

+            await this.protectedRoomsConfig.loadProtectedRoomsFromConfig(this.config);
+            await this.protectedRoomsConfig.loadProtectedRoomsFromAccountData();
+            this.protectedRoomsConfig.getExplicitlyProtectedRooms().forEach(this.protectRoom, this);


Can't this bit be replaced with a single call to await this.resyncJoinedRooms()?

Nope, because resyncJoinedRooms doesn't do anything (returns early) when config.protectAllJoinedRooms is disabled.

Note: There could be a flag to resyncJoinedRooms to prevent this early return. Your call.

Yoric

That will make things much nicer!

Yoric · 2022-10-18T14:24:17Z

src/Mjolnir.ts

-    private unprotectedWatchedListRooms: string[] = [];
-    public readonly protectedRoomsTracker: ProtectedRooms;
+
+    private protectedRoomsConfig: ProtectedRoomsConfig;


Yoric · 2022-10-19T07:44:23Z

src/Mjolnir.ts

+        // We filter out all policy rooms so that we only protect ones that are
+        // explicitly protected, so that we don't try to protect lists that we are just watching.
+        const filterOutManagementAndPolicyRooms = (roomId: string) => {
+            const policyListIds = this.policyLists.map(list => list.roomId);


Why do we rebuild policyListsId at every call?

Literally: because Array.prototype.includes doesn't accept a key function where i can put list.roomId instead.

Really: I just find it much simpler than tracking yet another group/category of rooms. I don't think it is costing us much to do this either.

src/Mjolnir.ts

Yoric · 2022-10-19T07:51:18Z

src/Mjolnir.ts

@@ -506,13 +485,7 @@ export class Mjolnir {

    public async warnAboutUnprotectedPolicyListRoom(roomId: string) {
        if (!this.config.protectAllJoinedRooms) return; // doesn't matter
-        if (this.explicitlyProtectedRoomIds.includes(roomId)) return; // explicitly protected
-
-        const createEvent = new CreateEvent(await this.client.getRoomStateEvent(roomId, "m.room.create", ""));


Do you understand what that was for?
I don't see any similar check in your new code, right? Should there be such a check?

Yes, we remove this check because it's not an accurate way to determine whether Mjolnir should protect the policy room. Especially if another Mjolnir/user created it, or if some other Mjolnir is now responsible for that list.

I mentioned it in the description of the PR too

We replace it with needing policy rooms to be explicitly protected in all circumstances and by showing you which policy rooms are protected distinctly when you use the status command (screenshot attached in PR too)

We also still warn when you first watch a policy room and it is unprotected.

src/Mjolnir.ts

src/ProtectedRoomsConfig.ts

Yoric · 2022-10-19T08:21:28Z

src/commands/ListProtectedRoomsCommand.ts

@@ -24,7 +24,7 @@ export async function execListProtectedRooms(roomId: string, event: any, mjolnir
    let text = `Protected rooms (${rooms.length}):\n`;

    let hasRooms = false;
-    for (const protectedRoomId in rooms) {
+    for (const protectedRoomId of rooms) {


Ouch, good catch!
How did we let that pass?

Yep, this was unfortunate.

Yoric · 2022-10-19T08:21:49Z

src/commands/StatusCommand.ts

-    if (mjolnir.lists.length === 0) {
-        html += "<li><i>None</i></li>";
-        text += "* None\n";
+    // FIXME: Make an issue to rename protectedRoomsTracker to just protectedRooms.


Please do :)

Yoric · 2022-10-19T08:22:21Z

src/commands/StatusCommand.ts

+    const subscribedLists = mjolnir.lists.filter(list => !mjolnir.explicitlyProtectedRooms.includes(list.roomId));
+    renderPolicyLists("Subscribed policy lists", subscribedLists);
+    const subscribedAndProtectedLists = mjolnir.lists.filter(list => mjolnir.explicitlyProtectedRooms.includes(list.roomId));
+    renderPolicyLists("Subscribed and protected policy lists", subscribedAndProtectedLists);


Nicer, thanks!

test/integration/protectedRoomsConfigTest.ts

The combination of `resyncJoinedRooms`, `unprotectedWatchedListRooms`, `explicitlyProtectedRoomIds`, `protectedJoinedRoomIds` was incomprehensible. #370 Separating out the management of `explicitlyProtectedRoomIds`, then making sure all policy lists have to be explicitly protected (in either setting of `config.protectAllJoinedRooms`) will make this code much much simpler. We will later change the `status` command to explicitly show which lists are watched and which are watched and protected.

#370

#370.

Gnuxie force-pushed the gnuxie/protected-rooms branch 2 times, most recently from 0b81b8b to ed59727 Compare October 17, 2022 10:36

Gnuxie marked this pull request as ready for review October 17, 2022 10:44

Gnuxie requested a review from Yoric October 17, 2022 10:44

ShadowJonathan reviewed Oct 17, 2022

View reviewed changes

Gnuxie force-pushed the gnuxie/protected-rooms branch from ed59727 to 6009555 Compare October 18, 2022 15:30

Yoric approved these changes Oct 19, 2022

View reviewed changes

Gnuxie mentioned this pull request Oct 19, 2022

Rename mjolnir.protectedRoomsTracker to mjolnir.protectedRooms and try make it private to mjolnir. #391

Open

Gnuxie force-pushed the gnuxie/protected-rooms branch from 6009555 to e7b32c3 Compare October 19, 2022 11:35

Gnuxie added 4 commits October 19, 2022 12:37

Make Mjolnir use ProtectedRoomsConfig

b94eefe

#370

Change list creation command to also protect the new room

bdb9b36

#370

Status command can distinguish between protected and watched lists.

ddbeaca

#370

Gnuxie force-pushed the gnuxie/protected-rooms branch from e7b32c3 to 7986b0f Compare October 19, 2022 11:38

Gnuxie added 3 commits October 19, 2022 12:39

protected rooms config test

7bdc100

#370

Stop ProtectedRoomsSet verifying permissions for unprotected rooms.

ef335d8

Fix list rooms command improperly iterating protected rooms.

04b3bc6

Gnuxie force-pushed the gnuxie/protected-rooms branch from 7986b0f to 04b3bc6 Compare October 19, 2022 11:39

Update roomMemberTest for removal of mjolnir.protectedRooms object.

048891c

#370.

Gnuxie force-pushed the gnuxie/protected-rooms branch from 2155669 to 048891c Compare October 19, 2022 14:12

Gnuxie merged commit a970733 into main Oct 19, 2022

Gnuxie mentioned this pull request Nov 24, 2022

Mjolnir is persisting aliases to org.matrix.mjolnir.watched_lists. #435

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix `config.protectAllJoinedRooms` leaking into explicitly protected rooms #385

fix `config.protectAllJoinedRooms` leaking into explicitly protected rooms #385

Gnuxie commented Oct 14, 2022 •

edited

Loading

ShadowJonathan Oct 17, 2022

Gnuxie Oct 17, 2022

Yoric Oct 19, 2022

Yoric left a comment

Yoric Oct 18, 2022

Yoric Oct 19, 2022

Gnuxie Oct 19, 2022 •

edited

Loading

Yoric Oct 19, 2022

Gnuxie Oct 19, 2022

Gnuxie Oct 19, 2022

Gnuxie Oct 19, 2022 •

edited

Loading

Yoric Oct 19, 2022

Gnuxie Oct 19, 2022

Yoric Oct 19, 2022

Yoric Oct 19, 2022

fix config.protectAllJoinedRooms leaking into explicitly protected rooms #385

fix config.protectAllJoinedRooms leaking into explicitly protected rooms #385

Conversation

Gnuxie commented Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnuxie Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gnuxie Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix `config.protectAllJoinedRooms` leaking into explicitly protected rooms #385

fix `config.protectAllJoinedRooms` leaking into explicitly protected rooms #385

Gnuxie commented Oct 14, 2022 •

edited

Loading

Gnuxie Oct 19, 2022 •

edited

Loading

Gnuxie Oct 19, 2022 •

edited

Loading